-
-
Notifications
You must be signed in to change notification settings - Fork 263
4.2dev #3860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4.2dev #3860
Conversation
Fix: for frontend pages with Server Error 500 when accessed by guests or users with basic permissions
Formatting corrected
Fix: Among other things, fixes a bug when accessing search statistics in the backend.
📝 WalkthroughWalkthroughAdds a stub public method Changes
Sequence Diagram(s)(omitted — changes are small and do not introduce a multi-component control flow that requires visualization) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (1)
66-67: Consider adding an import statement for consistency.The functionality is correct, but using the fully qualified class name
\phpMyFAQ\Twig\Extensions\LanguageCodeTwigExtension::classis inconsistent with howTranslateTwigExtension(line 64) andPluginTwigExtension(line 71) are referenced. Both of those use imported class names.♻️ Proposed refactor for consistency
Add the import statement at the top of the file with the other extension imports:
use phpMyFAQ\Twig\Extensions\PluginTwigExtension; use phpMyFAQ\Twig\Extensions\TranslateTwigExtension; +use phpMyFAQ\Twig\Extensions\LanguageCodeTwigExtension; use Twig\Environment;Then update the registration to use the imported class:
- // Always add the LanguageCode Extension - $this->twigEnvironment->addExtension(new AttributeExtension(\phpMyFAQ\Twig\Extensions\LanguageCodeTwigExtension::class)); + // Always add the LanguageCode extension + $this->twigEnvironment->addExtension(new AttributeExtension(LanguageCodeTwigExtension::class));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
**/*.{ts,tsx,php}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes for strings
Files:
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
🧬 Code graph analysis (2)
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php (2)
phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php (1)
getUserGroups(249-252)phpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.php (1)
getUserGroups(293-324)
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (2)
phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php (1)
addExtension(251-254)phpmyfaq/src/phpMyFAQ/Twig/Extensions/LanguageCodeTwigExtension.php (1)
LanguageCodeTwigExtension(26-33)
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php
286-286: Avoid unused parameters such as '$userId'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: phpMyFAQ 8.4 Test on ubuntu-latest
- GitHub Check: phpMyFAQ 8.5 Test on ubuntu-latest
- GitHub Check: phpMyFAQ 8.6 Test on ubuntu-latest
🔇 Additional comments (1)
phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php (1)
279-289: Remove the incorrect interface compliance justification.The implementation correctly returns an empty array for BasicPermission. However,
getUserGroupsis not defined inPermissionInterface—it's a concrete method specific to the permission implementations. The$userIdparameter is unused because BasicPermission doesn't track user groups, not due to interface requirements.
An import statement has been added to ensure consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (1)
1-133: Critical: Fix formatting issues to pass the build.The GitHub Actions build is failing because
mago format --dry-rundetected formatting inconsistencies. Runmago formatlocally to automatically apply the required formatting changes before merging this PR.#!/bin/bash # Description: Preview what formatting changes are needed # Run mago format in dry-run mode to see what changes would be made mago format --dry-run phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
**/*.{ts,tsx,php}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes for strings
Files:
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php
[error] 1-1: mago format --dry-run indicated formatting changes are required. Process exited with code 1. Run 'mago format' to apply changes.
🔇 Additional comments (2)
phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (2)
67-68: No issues found. The LanguageCodeTwigExtension is correctly implemented and registered.The extension exists at
phpmyfaq/src/phpMyFAQ/Twig/Extensions/LanguageCodeTwigExtension.php, provides thegetFromLanguageCodefilter, and is backed by comprehensive test coverage (LanguageCodeTwigExtensionTest) that validates the filter functionality. The registration pattern correctly mirrorsTranslateTwigExtension, and the unconditional registration aligns with the PR objective to fix search statistics access.
27-27: LanguageCodeTwigExtension implementation verified and correct.The import statement is correctly structured, the
LanguageCodeTwigExtensionclass exists and is properly implemented to provide language code translation functionality. The extension is correctly registered in the Twig environment.
move fix to StatisticsSearchController
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (1)
32-33: Consider grouping and sorting imports by namespace.The import statements mix phpMyFAQ and Twig namespaces. PER Coding Style 3.0 recommends grouping imports by namespace and sorting alphabetically within each group.
♻️ Suggested import organization
Group phpMyFAQ imports together, then Symfony, then Twig:
use phpMyFAQ\Core\Exception; use phpMyFAQ\Enums\PermissionType; use phpMyFAQ\Pagination; use phpMyFAQ\Pagination\UrlConfig; use phpMyFAQ\Session\Token; +use phpMyFAQ\Twig\Extensions\LanguageCodeTwigExtension; use phpMyFAQ\Translation; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; +use Twig\Extension\AttributeExtension; use Twig\Error\LoaderError; -use phpMyFAQ\Twig\Extensions\LanguageCodeTwigExtension; -use Twig\Extension\AttributeExtension;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
**/*.{ts,tsx,php}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes for strings
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
**/phpMyFAQ/Controller/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/phpMyFAQ/Controller/**/*.php: Controllers should extend AbstractController
Create Controllers inphpmyfaq/src/phpMyFAQ/Controller/and extend AbstractController
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
🧬 Code graph analysis (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (3)
phpmyfaq/src/phpMyFAQ/Twig/Extensions/LanguageCodeTwigExtension.php (1)
LanguageCodeTwigExtension(26-33)phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (1)
addExtension(94-97)phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php (1)
addExtension(251-254)
🔇 Additional comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (1)
46-46: No changes needed—extension registration follows the established codebase pattern.The per-action registration of
LanguageCodeTwigExtensionis correct and consistent. Global extensions inTwigWrapperare reserved for always-needed ones likeTranslateTwigExtension. ThegetFromLanguageCodefilter is only used in three templates, each served by a controller that explicitly registers the extension before rendering. This targeted approach is more efficient than global registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (1)
45-47: Consider registering the Twig extension in the constructor or a setup method to avoid per-action registration.While the current implementation works correctly, registering extensions in the constructor (or an initialization hook) is a cleaner architectural pattern than adding them within action methods. This approach makes extension registration explicit and centralized per controller instance, improving maintainability even if duplication isn't a practical concern with per-request controller instantiation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
**/*.{ts,tsx,php}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes for strings
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
**/phpMyFAQ/Controller/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/phpMyFAQ/Controller/**/*.php: Controllers should extend AbstractController
Create Controllers inphpmyfaq/src/phpMyFAQ/Controller/and extend AbstractController
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
🧠 Learnings (1)
📚 Learning: 2026-01-11T12:57:59.856Z
Learnt from: CR
Repo: thorsten/phpMyFAQ PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T12:57:59.856Z
Learning: Applies to **/phpMyFAQ/Controller/**/*.php : Controllers should extend AbstractController
Applied to files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php
🧬 Code graph analysis (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (3)
phpmyfaq/src/phpMyFAQ/Twig/Extensions/LanguageCodeTwigExtension.php (1)
LanguageCodeTwigExtension(26-33)phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (1)
addExtension(94-97)phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php (1)
addExtension(251-254)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: phpMyFAQ 8.5 Test on ubuntu-latest
- GitHub Check: phpMyFAQ 8.4 Test on ubuntu-latest
- GitHub Check: phpMyFAQ 8.6 Test on ubuntu-latest
🔇 Additional comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/StatisticsSearchController.php (1)
28-33:Twig\Extension\AttributeExtensiondoes not exist in twig/twig v3.10.0.
Lines 33 and 46 will fail at runtime. The AttributeExtension class was added in Twig 3.21; attempting to import or instantiate it against v3.10.0 will cause a fatal error. Update the Twig dependency version or remove this extension usage.⛔ Skipped due to learnings
Learnt from: CR Repo: thorsten/phpMyFAQ PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T12:57:59.856Z Learning: Applies to **/phpMyFAQ/Controller/**/*.php : Controllers should extend AbstractControllerLearnt from: CR Repo: thorsten/phpMyFAQ PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T12:57:59.856Z Learning: Applies to **/phpMyFAQ/Controller/**/*.php : Create Controllers in `phpmyfaq/src/phpMyFAQ/Controller/` and extend AbstractController
add the LanguageCode Extension in the TwigWrapper.php to fix a failure when accessing search statistics in the backend
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.